-
Notifications
You must be signed in to change notification settings - Fork 89
bump aws sdk v2 to v3 #2254
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bump aws sdk v2 to v3 #2254
Conversation
Hello sylvainsenechal,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
d8b16cc
to
6c6728b
Compare
fea7c95
to
e60add3
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
e1dc391
to
9ba51a2
Compare
bcdd21f
to
e791efe
Compare
667a273
to
54ba63d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats for the work done. Not so easy to switch from callback to async/await and the code is clean and I didn't detect some logic issue. 👏
For eslint / or syntax I didn't add a comment on all places where is it, but if you fix one, check other occurences
tests/zenko_tests/node_tests/smoke_tests/vault_admin_and_IAM_API_tests/iamApi.js
Show resolved
Hide resolved
tests/zenko_tests/node_tests/smoke_tests/vault_admin_and_IAM_API_tests/adminApi.js
Show resolved
Hide resolved
{ Bucket: bucket, Key: key }, | ||
)); | ||
const chunks = []; | ||
// eslint-disable-next-line no-restricted-syntax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, a comment to explain why you don't follow our linter and/or change it to make it compliant with it 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say this is not really useful, as the goal is to remove all these files and only use CTST
tests/zenko_tests/node_tests/iam_policies/cloudserver/AssumeRole.js
Outdated
Show resolved
Hide resolved
tests/zenko_tests/node_tests/iam_policies/cloudserver/AssumeRole.js
Outdated
Show resolved
Hide resolved
tests/zenko_tests/node_tests/cloudserver/keyFormatVersion/tests/versioningSuspended.js
Show resolved
Hide resolved
tests/zenko_tests/node_tests/cloudserver/keyFormatVersion/tests/versioningSuspended.js
Outdated
Show resolved
Hide resolved
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following reviewers are expecting changes from the author, or must review again: |
8ddae12
to
55f651a
Compare
}, () => cb(err)); | ||
})).then(() => cb(err)).catch(abortErr => { | ||
const aggregateError = new Error(`Original error: ${err}; Abort failed: ${abortErr}`); | ||
aggregateError.originalError = err; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we do that now ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhh, just a way to keep track of both error, not the cleanest but also not super important as its a test.
Here, we keep track of both the error from completeMultiPartUpload, and the error from abortMultiPartUpload
tests/zenko_tests/node_tests/iam_policies/cloudserver/AssumeRole.js
Outdated
Show resolved
Hide resolved
tests/zenko_tests/node_tests/iam_policies/cloudserver/AssumeRole.js
Outdated
Show resolved
Hide resolved
tests/zenko_tests/node_tests/iam_policies/cloudserver/AssumeRole.js
Outdated
Show resolved
Hide resolved
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
next => sourceClient.getObject(versionId, err => { | ||
assert.strictEqual(err.code, 'InvalidObjectState'); | ||
assert.strictEqual(err.statusCode, 403); | ||
const errorCode = err.Code || err.code; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we know in advance what the code field will be? or is it to differentiate between different error types (but should be the same, as we use the same client?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I can, I'll clean this
55f651a
to
ef5f304
Compare
/approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
Queue build failedThe corresponding build for the queue failed:
Remove the pull request from the queue
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue ZENKO-5054. Goodbye sylvainsenechal. |
Issue: ZENKO-5054